-
Notifications
You must be signed in to change notification settings - Fork 571
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
examples: add blecent and bleprph similar to mynewt-nimble apps #2807
base: master
Are you sure you want to change the base?
Conversation
These example apps create a BLE central and peripheral based on mynewt-nimBLE and work in companion. The apps are mostly based on their equivalents, which can be found at https://github.com/apache/mynewt-nimble/tree/master/apps
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @maxikrie :-) Please fix the code formatting issues: https://github.com/apache/nuttx-apps/actions/runs/11602988992/job/32309359887?pr=2807 :-)
examples/nimble_blecent/misc.c
Outdated
void | ||
print_bytes(const uint8_t *bytes, int len) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maxikrie I think it can fit in a single line
examples/nimble_blecent/misc.c
Outdated
} | ||
|
||
void | ||
print_mbuf(const struct os_mbuf *om) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
examples/nimble_blecent/misc.c
Outdated
while (om != NULL) { | ||
if (colon) { | ||
printf(":"); | ||
} else { | ||
colon = 1; | ||
} | ||
print_bytes(om->om_data, om->om_len); | ||
om = SLIST_NEXT(om, om_next); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please run the nxstyle or checkpatch.sh, it doesn't adhere to coding style, more info:
https://nuttx.apache.org/docs/latest/contributing/index.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will make changes and adhere to the standard. I am sorry that I didn't follow it immediately, it has been a while since my last pull request and I didn't see the documentation on contributing beforehand.
} | ||
|
||
char * | ||
addr_str(const void *addr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
examples/nimble_blecent/misc.c
Outdated
} | ||
|
||
void | ||
print_uuid(const ble_uuid_t *uuid) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
why not modify the current nimble peripheral example (https://github.com/apache/nuttx-apps/blob/master/examples/nimble/nimble_main.c)? do we need 2 different examples for this? |
I was debating this with myself. As stated, I based them on the mynewt-nimble apps and hence the peripheral is quite different from the existing one. I could, of course, modify the existing one losing the tight connection to the mynewt-nimble app. The other options are maintaining the existing one along side my contribution for legacy reasons, or purging the existing one (my preference). What do you think? @raiden00pl |
@maxikrie The Mynewt coding standard is far from the NuttX coding standard, so the code will be a lot different anyway. Keeping 2 examples demonstrating the same functionality seems pointless to me. The easiest thing to do would be to change the name of existing |
CI checkpatch is now fixed, CI restarted :-) |
Source code still needs reformat, @maxikrie please take a look at: |
@cederom Is there a way to check whether my changes will pass the check before committing? I read the documentation on pre-commit, but it seems that this only applies to the nuttx repository - at least this repository is missing the required .pre-commit-config.yaml. I have installed pre-commit in the nuttx repository and tested it on branch nuttx-12.7.0, where everything is fine. I then naively tried from within the nuttx repository: |
please run the following command to check the style: |
ping @maxikrie |
I fixed the formatting issues and fixed an error in the Makefile (I previously had only tested the CMake build), but there are again failed checks. Can somebody check? I am not sure what to make of them.. @xiaoxiang781216 @cederom |
Thank you @maxikrie :-) Most of the formatting issues detected by CI are in the codebase already: Looks like these still needs an update?
|
I am not sure, should I look into this? I am just checking since I didn't touch these and they didn't appear to be an issue when I run the nxstyle tool. I want to make sure this is not an issue with the CI. |
return buf; | ||
} | ||
|
||
void print_uuid(const ble_uuid_t *uuid) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all pointers in must be marked with FAR
: FAR const ble_uuid_t *uuid)
/** | ||
* Utility function to log an array of bytes. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NuttX doesn't use Doxygen comments. This and all similar should be converted to:
/* Utility function to log an array of bytes */
* @param event The type of event being signalled. | ||
* @param ctxt Various information pertaining to the event. | ||
* @param arg Application-specified argument; unuesd by | ||
* bleprph. | ||
* | ||
* @return 0 if the application successfully handled the | ||
* event; nonzero on failure. The semantics | ||
* of the return code is specific to the | ||
* particular GAP event being signalled. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doxygen is not allowed. The correct comment block for function looks like this:
/****************************************************************************
* Private Functions
****************************************************************************/
/****************************************************************************
* Name: <Static function name>
*
* Description:
* Description of the operation of the static function.
*
* Input Parameters:
* A list of input parameters, one-per-line, appears here along with a
* description of each input parameter.
*
* Returned Value:
* Description of the value returned by this function (if any),
* including an enumeration of all possible error values.
*
* Assumptions/Limitations:
* Anything else that one might need to know to use this function.
*
****************************************************************************/
does anyone have any idea why CI is returning nxstyle errors unrelated to PR content? What is this |
@maxikrie Could you try rebasing with the latest Also the CI Check seems to be running OK for the latest builds. Thanks! https://github.com/apache/nuttx-apps/actions/workflows/check.yml |
Summary
These example apps create a BLE central and peripheral based on mynewt-nimBLE and work in companion. The apps are mostly based on their equivalents, which can be found at https://github.com/apache/mynewt-nimble/tree/master/apps
Impact
Testing
These were successfully tested on two nrf52840-dk boards.